MPC consolidation #64
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cpatino-intive
force-pushed
the
WP-5170/mpc-consolidation-refactor
branch
from
3f959f5 to
87d9bd8
Compare
Contributor
pranavjain97
left a comment
pranavjain97
left a comment
There was a problem hiding this comment.
Overall lgtm; small nits
| throw new Error('consolidateAddresses must be an array of addresses'); | ||
| } | ||
|
|
||
| const isMPC = wallet._wallet.multisigType === 'tss'; |
Contributor
There was a problem hiding this comment.
Suggested change
| const isMPC = wallet._wallet.multisigType === 'tss'; | |
| const isMPC = wallet.multisigType() === 'tss'; |
| msg = `Consolidations failed: ${failedTxs.length} and succeeded: ${successfulTxs.length}`; | ||
| } else { | ||
| // All failed | ||
| status = 400; |
Contributor
There was a problem hiding this comment.
this should be a 500. 400 is if it fails due to an error from user input
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the account consolidation flow to support both onchain multisig and MPC (TSS) wallets, introduces a commonKeychain parameter, and updates the API spec and tests.
- Add optional
commonKeychainto the ConsolidateRequest spec and validate it in handlerUtils. - Refactor
handleConsolidateto loop through builds, collect per-build success/failure, and branch betweensendAccountConsolidationandsignAndSendTxRequests. - Export
signAndSendMultisig, update API JSON (masterBitgoExpress.json) with a newaccelerateendpoint, and adapt tests for the new consolidation logic.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/api/master/routers/masterApiSpec.ts | Added commonKeychain as an optional field in ConsolidateRequest |
| src/api/master/handlers/handleSendMany.ts | Made signAndSendMultisig an exported function |
| src/api/master/handlers/handleConsolidate.ts | Complete overhaul to handle MPC vs. multisig consolidation and result handling |
| src/api/master/handlerUtils.ts | Extended getWalletAndSigningKeychain to accept and validate commonKeychain |
| src/tests/api/master/consolidate.test.ts | Updated and expanded tests for both multisig and MPC consolidation flows |
| masterBitgoExpress.json | Added /accelerate endpoint and included commonKeychain in spec |
Comments suppressed due to low confidence (5)
src/api/master/handlerUtils.ts:20
- Update the JSDoc/comments for
getWalletAndSigningKeychainto describe the newcommonKeychainparameter and its purpose.
params: { source: 'user' | 'backup'; pubkey?: string; commonKeychain?: string };
src/tests/api/master/consolidate.test.ts:12
- The test suite title references
consolidateunspentsbut the endpoint under test is/consolidate; please correct the describe string to match the actual route.
describe('POST /api/:coin/wallet/:walletId/consolidateunspents', () => {
src/api/master/handlers/handleConsolidate.ts:38
- [nitpick] The request spec marks
consolidateAddressesas optional, but this validation rejects undefined values. Decide whether missing addresses should default to full consolidation or update the spec to make it required.
throw new Error('consolidateAddresses must be an array of addresses');
masterBitgoExpress.json:9
- [nitpick] The new
accelerateendpoint appears unrelated to MPC consolidation; consider splitting it into its own PR or providing additional context.
"/api/{coin}/wallet/{walletId}/accelerate": {
src/api/master/handlers/handleSendMany.ts:195
- [nitpick] The
signAndSendMultisigfunction was made exported but isn’t referenced elsewhere; remove the export if it’s unused or update imports to leverage it where appropriate.
export async function signAndSendMultisig(
mohammadalfaiyazbitgo
approved these changes
Jul 14, 2025
Contributor
mohammadalfaiyazbitgo
left a comment
mohammadalfaiyazbitgo
left a comment
There was a problem hiding this comment.
please address comments in a follow up.
Comment on lines
+116
to
+119
| (error as any).result = { | ||
| success: successfulTxs, | ||
| failure: failedTxs, | ||
| }; |
Contributor
There was a problem hiding this comment.
please avoid any when you can.
pranavjain97
approved these changes
Jul 14, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MPC consolidation
TASK: WP-5170